Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add eio backend for ocaml-dns client #312

Open
wants to merge 5 commits into
base: main
Choose a base branch
from
Open

Conversation

bikallem
Copy link

@bikallem bikallem commented Jun 23, 2022

This PR adds support for eio backend for ocaml-dns client. At the moment it only adds tcp tansport protocol support. I intend to add tls or DNS over TLS support in the future once eio support for ocaml-tls is done.

This PR has a few unreleased packages dependencies:

At the moment eio doesn't support monotonic clock, so we still use mtime. It may be preferable to use monotonic clock support from eio once support for it lands in eio (ocaml-multicore/eio#229). However, that should be a future PR.

@bikallem bikallem changed the title Add eio as one of the backends for ocaml-dns client Add eio backend for ocaml-dns client Jun 23, 2022
@bikallem bikallem marked this pull request as ready for review June 24, 2022 12:21
Copy link
Contributor

@talex5 talex5 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I experimented with some API changes here: https://github.com/bikallem/ocaml-dns/compare/eio...talex5:ocaml-dns:eio?expand=1

The main ones are:

  1. Instead of providing a run function that just calls mirage-crypto, it's just as easy for the user call it themselves (and better if they're using crypto for other purposes).
  2. There's no need to pass Client as a first-class module. We know what it is statically.
  3. I moved create to create_from_env for people who want that, and added a more explicit create function so people can pass the inputs separately.

This changes the user code from:

  Eio_main.run @@ fun env -> 
  Eio.Switch.run @@ fun sw -> 
  Dns_client_eio.run env @@ fun (module Client) ->   
  let env = (env :> Dns_client_eio.env) in
  let c = Client.create (env, sw) in

to

  Eio_main.run @@ fun env -> 
  Mirage_crypto_rng_eio.run (module Mirage_crypto_rng.Fortuna) env @@ fun () ->
  Eio.Switch.run @@ fun sw -> 
  let c = Client.create_from_env ~sw env in

The needs to specify Fortuna explicitly seems a bit ugly to me, but if we don't want that then it should be fixed in mirage-crypto.

(Was the idea of the first class module to force users to initialise mirage-crypto before using the library? If so, we could probably fix this more easily by having Mirage_crypto_rng_eio.run return a token and passing that to create_from_env. Passing first-class modules around tends to get awkward.)

I'm not sure about passing resolv.conf as an Eio.Path. Possibly it should just be a function unit -> string. Also, Eio should provide some convenience types for paths (maybe Path.rw and Path.ro or something).

@bikallem
Copy link
Author

bikallem commented Oct 4, 2022

I have rebased/updated the PR to address reviewer feedback.

(Was the idea of the first class module to force users to initialise mirage-crypto before using the library? If so, we could probably fix this more easily by having Mirage_crypto_rng_eio.run return a token and passing that to create_from_env. Passing first-class modules around tends to get awkward.)

Indeed, the idea of the API was to maintain the invariant that that consumers of the library to be able to use the module only when run under Mirage_crypto_rng_eio.run. The current API aims to maintain that invariant as well as be convenient such that one doesn't have to manually run (although they can if they so desire) the mirage crypto rng generator.

@bikallem
Copy link
Author

bikallem commented Oct 4, 2022

Hmmm ... not sure why the ci is failing. Can anyone help?

@talex5
Copy link
Contributor

talex5 commented Oct 4, 2022

It says:

lru.0.3.0: Requires ocaml >= 4.03.0 & < 5.0

@hannesm
Copy link
Member

hannesm commented Oct 25, 2022

I'm not particularly a fan of passing first-class modules around, and don't think there's a good reason here.

I wonder what the "EIO" and "RANDOM" story is (if there's any):

  • I have seen some work on mirage-crypto-rng-eio, so you can initialize and feed the Fortuna RNG;
  • There has been as well some support for getrandom (but seeing the discussion of Fix getrandom(2) ocaml-multicore/eio#344 I'm not sure whether this can be safely used) -- this is exposed as "secure_random" by the "StdEnv" AFAICT (though I'm not sure what else is in this "StdEnv" and why);
  • And OCaml in 5.0 has support for getentropy to feed its internal RNG (Random module).

So, what is the plan for EIO and RANDOM? How and who should decide where to take random numbers from? Is this all up to the user (and do they have a unified interface)? In DNS, we don't need cryptographically secure random, something that is not predictable is sufficient (it's used mainly for the ID anyways, to avoid spoofing of DNS packets (see https://en.wikipedia.org/wiki/DNS_spoofing if interested in details)).

@talex5
Copy link
Contributor

talex5 commented Oct 27, 2022

I'm not particularly a fan of passing first-class modules around, and don't think there's a good reason here.

Indeed. I removed it in my branch (see #312 (review)).

(but seeing the discussion of ocaml-multicore/eio#344 I'm not sure whether this can be safely used)

The PR is just trying to make it harder to misuse the API. mirage-crypto-rng-eio already uses it correctly (I'm not aware of anyone using it incorrectly).

In DNS, we don't need cryptographically secure random, something that is not predictable is sufficient

I assume it's just copying the existing lwt code, which uses mirage-crypto-rng.lwt. Would be good to reduce the dependencies if it's not needed :-)

@hannesm
Copy link
Member

hannesm commented Oct 27, 2022

Trying once more. In eio, do you have a user guide / approach to randomness?

I can see three different things going on here:

  • the OCaml 5 runtime (Stdlib.Random -- which is a LXM: Better Splittable Pseudorandom Number
    Generators (and Almost as Fast); seeding is done via getentropy() if available)
  • the eio providing a "secure_random" in "StdEnv", which calls out to getrandom()
  • mirage-crypto-rng-eio calling out to rdrand/rdseed and getrandom/getentropy (periodic task) feeding a Fortuna RNG

Now, my questions are:

  • is there a unified interface between these implementations?
  • is there an eio-recommended usage of either of these?
  • is the approach to have one syscall (getentropy/getrandom) each time some random is needed too expensive?

Maybe this PR here is the wrong context for these questions and considerations, but for me it is important to understand how "eio" should be used in order to avoid having to back out such changes.

@bikallem
Copy link
Author

bikallem commented Oct 27, 2022

I have now removed the use of first class modules, the ohost.exe included in the PR don't seem to be working after latest rebase and commit, so I debugging through the issue.

EDIT: Updating the default nameservers to the same as unix client did the trick. It is working now.

Add `dns-client-eio` opam package - an eio backe-end for
dns-client. It is compatible with OCaml version 5.0 and above.
@bikallem
Copy link
Author

Needs mirleft/ocaml-tls#458 to operate correctly.

@hannesm
Copy link
Member

hannesm commented Feb 15, 2023

So it seems we're a bit stuck on this PR:

How to move forward here? I unlikely have time before Q3 2023 to look into "eio", but I suspect you'd like to have "something merged" sooner. But I'm still overwhelmed by the complexity of eio and don't feel confident to grasp the semantics of what is happening where (and why concurrently etc.).

@bikallem
Copy link
Author

bikallem commented Mar 6, 2023

@hannesm This PR is now ready. It doesn't have the TLS issue anymore.

(a) there's some lack of a "random" story for eio (and it seems nobody is pushing that forward (no, it won't be me))

The issue is separate from dns-client-eio and should be handled in the eio repo.

@dinosaure
Copy link
Member

I take a long look about this PR and it seems that DNS request by another domain seems not an option with the current proposed design in this PR - at least, the use of "mutable" without protection makes me think there could be problems.

The reason I point to this specific usage is that it seems to me that the design of happy-eyeballs (the core package, not the happy-eyeballs-lwt) is to be a background task that maintains a "pool" of TCP/IP connections to nameservers and can resolve a DNS request from the user. It is therefore "legitimate" to think that one domain would take care of such a task in the background while the user application could interact with it from another domain to resolve domain-name.

@bikallem
Copy link
Author

bikallem commented Jul 6, 2023

I take a long look about this PR and it seems that DNS request by another domain seems not an option with the current proposed design in this PR - at least, the use of "mutable" without protection makes me think there could be problems.

In current eio, IIUIC resource/connection/switch created in one domain can't be moved/used in another domain. Therefore, even if happy eyeballs is able to create connections, I am not sure if those connections can be used/shared in an ad-hoc manner by the domains. However, this does not preclude having multiple/parallel dns client requests via OCaml domains - which the current design offers/enables.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants